Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RSDK-5903 Add micro-rdk support for HC-SR04 type ultrasonic sensors #136

Merged
merged 15 commits into from
Jan 9, 2024

Conversation

acmorrow
Copy link
Member

@acmorrow acmorrow commented Jan 5, 2024

No description provided.

@acmorrow acmorrow requested a review from a team as a code owner January 5, 2024 15:28
@acmorrow
Copy link
Member Author

acmorrow commented Jan 5, 2024

This is working well enough that I thought it was worth getting a review started. There are a few small TODOs left, and a few other things I'll leave comments about. For now, this doesn't use the mcpwm subsystem, it just manually times the pulse based on digital interrupts. I'd suggest that if we want to move to a mcpwm based approach that we land this implementation first in order to unblock using this sensor with the micro-rdk.

src/esp32/hcsr04.rs Show resolved Hide resolved
src/esp32/hcsr04.rs Outdated Show resolved Hide resolved
src/esp32/hcsr04.rs Outdated Show resolved Hide resolved
src/esp32/hcsr04.rs Outdated Show resolved Hide resolved
src/esp32/hcsr04.rs Outdated Show resolved Hide resolved
src/esp32/hcsr04.rs Outdated Show resolved Hide resolved
src/esp32/hcsr04.rs Show resolved Hide resolved
src/esp32/hcsr04.rs Outdated Show resolved Hide resolved
Copy link
Member

@npmenard npmenard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done with first pass

src/common/sensor.rs Outdated Show resolved Hide resolved
src/esp32/hcsr04.rs Show resolved Hide resolved
src/esp32/hcsr04.rs Show resolved Hide resolved
src/esp32/hcsr04.rs Show resolved Hide resolved
src/esp32/hcsr04.rs Show resolved Hide resolved
src/esp32/hcsr04.rs Outdated Show resolved Hide resolved
src/esp32/hcsr04.rs Show resolved Hide resolved
src/esp32/hcsr04.rs Show resolved Hide resolved
@acmorrow
Copy link
Member Author

acmorrow commented Jan 8, 2024

Meanwhile, I'm getting a clippy error I don't totally understand:

error: unused `#[macro_use]` import
  --> src/lib.rs:12:13
   |
12 | #[macro_use(defer)]
   |             ^^^^^
   |
   = note: `-D unused-imports` implied by `-D warnings`

error: could not compile `micro-rdk` (lib) due to previous error
make: *** [Makefile:62: clippy-native] Error 101
Error: Process completed with exit code 2.

I really am using the defer macro though, so I'm not sure why it is complaining. However, if I try to move the

#[macro_use(defer)]
extern crate scopeguard;

block into hcsr04.rs it fails to compile:

error: could not compile `micro-rdk` (lib) due to 2 previous errors
warning: build failed, waiting for other jobs to finish...
error[E0468]: an `extern crate` loading macros must be at the crate root
  --> /host/micro-rdk/src/esp32/hcsr04.rs:44:1
   |
44 | extern crate scopeguard;
   | ^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0468`.
make: *** [Makefile:11: build-esp32-bin] Error 101

So, I'm not sure where I should be doing this (or doing something else).

@acmorrow
Copy link
Member Author

acmorrow commented Jan 8, 2024

Meanwhile, I'm getting a clippy error I don't totally understand:

error: unused `#[macro_use]` import
  --> src/lib.rs:12:13
   |
12 | #[macro_use(defer)]
   |             ^^^^^
   |
   = note: `-D unused-imports` implied by `-D warnings`

error: could not compile `micro-rdk` (lib) due to previous error
make: *** [Makefile:62: clippy-native] Error 101
Error: Process completed with exit code 2.

I really am using the defer macro though, so I'm not sure why it is complaining. However, if I try to move the

#[macro_use(defer)]
extern crate scopeguard;

block into hcsr04.rs it fails to compile:

error: could not compile `micro-rdk` (lib) due to 2 previous errors
warning: build failed, waiting for other jobs to finish...
error[E0468]: an `extern crate` loading macros must be at the crate root
  --> /host/micro-rdk/src/esp32/hcsr04.rs:44:1
   |
44 | extern crate scopeguard;
   | ^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0468`.
make: *** [Makefile:11: build-esp32-bin] Error 101

So, I'm not sure where I should be doing this (or doing something else).

@gvaradarajan pointed out to me on slack that I hadn't guarded the macro(use) with #[cfg(feature = "esp32")], so that's fixed now.

Copy link
Member

@gvaradarajan gvaradarajan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wanted a minor change to the doc comment, but pending the executor conversation LGTM

src/esp32/hcsr04.rs Outdated Show resolved Hide resolved
Copy link
Member

@npmenard npmenard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@acmorrow
Copy link
Member Author

acmorrow commented Jan 9, 2024

I filed two new tickets and added TODO's referencing them:

@npmenard npmenard force-pushed the main branch 2 times, most recently from 08e49bb to 51112dc Compare January 9, 2024 15:31
@acmorrow acmorrow merged commit a17f70e into viamrobotics:main Jan 9, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants